Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use app router template #9186

Open
wants to merge 27 commits into
base: v-next
Choose a base branch
from
Open

Use app router template #9186

wants to merge 27 commits into from

Conversation

gautamsi
Copy link
Member

@gautamsi gautamsi commented Jun 25, 2024

ref #9183

Blocked by

This PR updates the template to generate the App router, default to typescript version which can be turned off by setting config.ui.tsx = false

What is completed/Verified:

  • generate app routes in app/(admin) folder (creates admin route specific layout page. if src page exist, it generates it in `src/app/(admin)
  • Stop generating next.config.js file, this should be generated from create-keystone-app going forward (TODO)
  • set default ui.basePath to /admin can set ui.basePath to /admin or any other sub path
  • No longer copies admin files or generates pages export copy as they are no longer needed
  • Does not clear the admin folder files if exist, only overwrites the special config files which contains up to date admin meta and view cache, this should be removed before GA
  • Verified custom pages
  • Verified custom fields and field views
  • can use relative path or even "paths" from typescript (@fields/text/myView)
  • Added --reset-admin flag to dev command to force clean (admin) folder, useful in regenerate the admin template if updated in future. should never be needed except braking upgrade
  • fix build and start script
  • @keystone-6/auth templates
  • page middleware

Pending:

  • Documentation
  • fixing final set of failing tests

@dcousens can you create a v-next branch which should be actual target for these PRs instead of main

@gautamsi gautamsi force-pushed the #9183#1 branch 4 times, most recently from 29477ea to 19ab9c5 Compare June 25, 2024 06:08
@dcousens dcousens changed the base branch from main to v-next June 25, 2024 06:31
Copy link

codesandbox-ci bot commented Jun 25, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 3e6d294:

Sandbox Source
@keystone-6/sandbox Configuration

@gautamsi
Copy link
Member Author

gautamsi commented Jun 25, 2024

@dcousens I will need some help in fixing tests,
* smoke tests never finishes
* wired error in package unit tests, it has mismatch on keystone error message in build but unable to trace.
* random test failure with checkForTooManyEngines prisma error

I have disabled some ui tests, they need fixes but we can do that before merging to main

@dcousens I have fixed most test cases, these tests definitely helped me fix a lot of issues.

I see that document-field test can not be fixed due to static generation, I may have missed something.

Test live-reloading.test is not behaving properly, same thing, trying to fix some page router vs app router thing. I am having trouble debugging locally on windows for some reason.

@dcousens
Copy link
Member

dcousens commented Jun 25, 2024

@gautamsi I'm super busy and can't engage on this this week, but, I'll wrap back around as soon as I can and help move this forward 💛

@gautamsi gautamsi force-pushed the #9183#1 branch 14 times, most recently from 303b060 to f86b3fd Compare June 30, 2024 12:31
@gautamsi
Copy link
Member Author

gautamsi commented Jun 30, 2024

@dcousens I have not generated default files for all the example/test projects, have modified the code as such it will generate all the required nextjs files for dev and build. User would need to commit them in git.

The file generation is one time except the .admin/index file which generates the admin meta has and static keystone props for admin.

isLiveReload: boolean
) {
// when we're not doing a live reload, we want to clear everything out except the .next directory (not the .next directory because it has caches)
// so that at least every so often, we'll clear out anything that the deleting we do during live reloads doesn't (should just be directories)
if (!isLiveReload) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part of live reload is not needed, we do not even need to figure out file linking from main dir to isolated dir anymore

@gautamsi
Copy link
Member Author

gautamsi commented Jul 7, 2024

@dcousens do you plan to check this out soon?

renovate bot added 2 commits July 23, 2024 10:46
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
@gautamsi
Copy link
Member Author

gautamsi commented Jul 29, 2024

! have tried to upgrade my large project to this, seems like lot of things to change.

I am going to update this PR with few more tweaks which will allow switching with app router but only client side rendering mostly.

for full server side, we will need design system to change as well.

@dcousens can you review #9189 and #9204 and merge to v-next so that I can fix this PR with final changes for this in v-next, I was thinking if we could release a beta tag when this merge in v-next to test further changes

@dcousens
Copy link
Member

Hi @gautamsi,
Sorry I'm being a bit slow on this - I'm not trying to block your work, I'm just short on time this week.

@gautamsi gautamsi mentioned this pull request Aug 5, 2024
@gautamsi
Copy link
Member Author

gautamsi commented Aug 7, 2024

@dcousens any traction this week?

@gautamsi
Copy link
Member Author

@dcousens please update v-next branch to main, I will rebase all the branches

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants